Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#507] waitset adjustments #509

Merged
merged 5 commits into from
Nov 12, 2024
Merged

[#507] waitset adjustments #509

merged 5 commits into from
Nov 12, 2024

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Nov 9, 2024 •
edited
Loading

Notes for Reviewer

  • DeadlineQueue::missed_deadlines() callback needs to return CallbackProgression so that the user is able to stop handling missed deadlines when required.
  • WaitSet::try_wait_and_process was renamed to WaitSet::wait_and_process_once since the function was actually blocking in contrast what the name suggested with the try_ prefix
    • behavior adjusted, blocks until the first event arrives, then handles all events and returns, to make it usable in external event loops.
  • WaitSet::wait_and_process and WaitSet::wait_and_process_once callbacks need to return CallbackProgression to terminate event handling whenever required - this replaces the buggy WaitSet::stop() call.

Pre-Review Checklist for the PR Author

  1. Add sensible notes for the reviewer
  2. PR title is short, expressive and meaningful
  3. Relevant issues are linked in the References section
  4. Every source code file has a copyright header with SPDX-License-Identifier: Apache-2.0 OR MIT
  5. Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  6. Commits messages are according to this guideline
  7. Tests follow the best practice for testing
  8. Changelog updated in the unreleased section including API breaking changes
  9. Assign PR to reviewer
  10. All checks have passed (except task-list-completed)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

Closes #507

@elfenpiff elfenpiff requested a review from orecham November 9, 2024 22:28
@elfenpiff elfenpiff self-assigned this Nov 9, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.29%. Comparing base (5f6a0dc) to head (71c4db4).
Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
iceoryx2/src/port/waitset.rs 78.94% 8 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #509      +/-   ##
==========================================
+ Coverage   79.19%   79.29%   +0.10%     
==========================================
  Files         200      200              
  Lines       23716    23757      +41     
==========================================
+ Hits        18781    18838      +57     
+ Misses       4935     4919      -16
Files with missing lines Coverage Δ
iceoryx2-bb/posix/src/deadline_queue.rs 89.43% <100.00%> (-0.22%) ⬇️
iceoryx2/src/port/waitset.rs 79.32% <78.94%> (+1.75%) ⬆️

... and 16 files with indirect coverage changes

orecham
orecham reviewed Nov 11, 2024
View reviewed changes
iceoryx2-ffi/cxx/src/waitset.cpp
auto run_callback(iox2_waitset_attachment_id_h attachment_id, void* context) {
auto* fn_call = internal::ctx_cast<iox::function<void(WaitSetAttachmentId<S>)>>(context);
fn_call->value()(WaitSetAttachmentId<S>(attachment_id));
auto run_callback(iox2_waitset_attachment_id_h attachment_id, void* context) -> iox2_callback_progression_e {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious - why is the C enum used? I guess it's because this is called directly by the C API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orecham this is just the internal implementation. I need a C style callback in the C++ API that has the C++ iox::function stored in context and can be used as a callback in the C API.

orecham
orecham reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@orecham orecham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question.

@elfenpiff elfenpiff requested a review from orecham November 11, 2024 21:35
@elfenpiff elfenpiff merged commit 5b8e6ee into eclipse-iceoryx:main Nov 12, 2024
46 of 47 checks passed
@elfenpiff elfenpiff deleted the iox2-507-waitset-adjustments branch November 12, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orecham orecham orecham approved these changes

Assignees

@elfenpiff elfenpiff

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WaitSet adjustments
2 participants
@elfenpiff @orecham